-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Configuration API #12301
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Configuration API #12301
Conversation
3b67abb
to
dcb7016
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 45 out of 46 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- paper-api-generator.settings.gradle.kts: Language not supported
Comments suppressed due to low confidence (3)
paper-api/src/main/java/org/bukkit/event/player/PlayerLoginEvent.java:22
- The deprecation version 'idk' is unclear; please update it to a proper version string to improve clarity.
@Deprecated(since = "idk")
paper-api/src/main/java/io/papermc/paper/event/connection/configuration/PlayerConfigurationConnectionEvent.java:8
- [nitpick] The field 'loginConnection' should be renamed to 'configurationConnection' to better reflect its purpose in a configuration event.
private final PlayerConfigurationConnection loginConnection;
paper-api/src/main/java/io/papermc/paper/connection/PlayerCommonConnection.java:3
- [nitpick] Consider clarifying the name of this interface to more clearly describe its purpose, such as 'PlayerSharedConnection' or a similar term.
// TODO: Naming?
paper-api/src/main/java/io/papermc/paper/connection/PlayerConfigurationConnection.java
Outdated
Show resolved
Hide resolved
import java.util.concurrent.CompletableFuture; | ||
|
||
@NullMarked | ||
public interface CookieConnection extends PlayerConnection { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PlayerCookieConnection
* | ||
* @return The IP address | ||
*/ | ||
InetAddress getAddress(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is kinda doubled between this and the login connection and then it isn't present on the game connection because player presumably has it.
While the profile may not be available, address information (Inet or Socket (hi emily)) feels very very basic?
InetAddress getAddress(); | ||
|
||
/** | ||
* Gets the raw address of the player logging in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dots at the end of jd bodies.
Goes for the rest of this PR too.
import java.util.concurrent.CompletableFuture; | ||
|
||
@NullMarked | ||
public interface CookieConnection extends PlayerConnection { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Experimental the living shit out of all of these types.
/** | ||
* Completes the configuration for this player, which will cause this player to reenter the game. | ||
* <p> | ||
* Note, this should be only be called if you are reconfiguring the player. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This, similar to #enterConfiguration poses the interesting issue of "ah, you just requested what is basically a destruction of this connection state, what happens to this instance".
Calling #clearChat after this is illegal.
Getters might be fine. But we should look into a proper way to signal, "you destroyed this connection instance".
import org.jetbrains.annotations.NotNull; | ||
|
||
/** | ||
* A task that allows you to configurate the player. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an event, not a task.
* <p> | ||
* This occurs after configuration, but before the player has entered the world. | ||
*/ | ||
public class AsyncPlayerConnectionConfigureEvent extends PlayerConfigurationConnectionEvent { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naming here indeed hits the wall a bit.
A "AsyncPlayerConnectionConfigureEvent", representing when a player is near-finished with their configuration phase, feels rough, the parent name "PlayerConfigurationConectionEvent" makes this even worse, either it is Configuration or Configure, pick one xD
The fact they are swapped also hurts a bit.
import org.jetbrains.annotations.NotNull; | ||
|
||
public class PlayerConnectionInitialConfigureEvent extends PlayerConfigurationConnectionEvent { | ||
private static final HandlerList handlers = new HandlerList(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and everywhere else, HANDLERS, or HANDLER_LIST.
import net.minecraft.network.chat.Component; | ||
|
||
-public record DisconnectionDetails(Component reason, Optional<Path> report, Optional<URI> bugReportLink) { | ||
+// Paper start |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
identifying comments. What API is this for.
import net.minecraft.util.profiling.Profiler; | ||
import org.slf4j.Logger; | ||
|
||
-public abstract class ServerCommonPacketListenerImpl implements ServerCommonPacketListener { | ||
+public abstract class ServerCommonPacketListenerImpl implements ServerCommonPacketListener, org.bukkit.craftbukkit.entity.CraftPlayer.TransferCookieConnection { // CraftBukkit | ||
+public abstract class ServerCommonPacketListenerImpl implements ServerCommonPacketListener{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Useless diff.
Please provide feedback on naming, and general event consensis.
But my goal is to have the following:
CookieConnection
)AsyncPlayerConnectionConfigurateEvent
)